FIX: Make positional arguments to LaplacianThickness require previous argument #2848

yarikoptic · 2019-01-11T20:55:00Z

I have no clue yet what needs to be done but here you go
Note: sits on top of #2846

Closes: #2847

TODOs:

figure out how nipype should be changed to get issue resolved
extend test to verify correct operation

…args This way makes it easier to test cmdline formation so we have 4.9 and not 4.90000

effigies · 2019-01-13T14:36:50Z

Please merge master to fix the pytest build.

effigies · 2019-01-16T19:52:09Z

I think the easiest approach is:

     prior_thickness = traits.Float(
         argstr='%f',
         desc='Prior thickness (defaults to 500)',
+        requires=['smooth_param'],
         position=5)
     dT = traits.Float(
         argstr='%f',
         desc='Time delta used during integration (defaults to 0.01)',
+        requires=['prior_thickness'],
         position=6)
     sulcus_prior = traits.Float(
         argstr='%f',
         desc='Positive floating point number for sulcus prior. '
              'Authors said that 0.15 might be a reasonable value',
+        requires=['dT'],
         position=7)
     tolerance = traits.Float(
         argstr='%f',
         desc='Tolerance to reach during optimization (defaults to 0.001)',
+        requires=['sulcus_prior'],
         position=8)

For the sake of better error messages, you might add all prior optional arguments to requires, but this should at least cascade in such a way as to throw some kind of error when someone tries to skip a parameter.

That said, I think it's perfectly reasonable for nipype to pass defaults, even if those in the underlying tool might change; it's better for provenance and makes nipype pipelines resilient to changes in defaults (if not to changes in effects of those parameters).

@effigies

* origin/master: (63 commits) Update nipype/interfaces/dipy/tracks.py Update nipype/interfaces/dipy/reconstruction.py MNT: Install numpy!=1.16.0 from conda in Docker Add FSL auto test remake specs Update nipype/interfaces/io.py Remove return type named tuple Update nipype/info.py STY: Whitespace, line length Remove out_ prefix from EddyQuad outputs Apply minor edits from code review Use os.path.basename for the fallback output_dir in EddyQuad._list_outputs() Add output_dir check to EddyQuad._list_outputs() Remove redundant out_avg_b_png lines Add glob stuff back in Edit in response to @effigies comments on PR nipy#2825 Fix tests for EddyQuad in interfaces/fsl/epi.py Add name to .zenodo.json Add doctest for EddyQuad fix: made the outputdir be mandatory and use the default val ...

…blem of positional args see e.g. nipy/nipype#2848

@effigies

Thanks @effigies. This should prevent incorrect specification` of parameters in the command line since for ants they are just positional ones, so all previous ones should be specified

yarikoptic · 2019-01-25T03:54:41Z

Thanks @effigies , sorry I've missed your recommendation!
Seems to work -

failure of gearificator test when only prior_thickness is set

$> gearificator --pdb spec process --regex '.*\.LaplacianTh' --run-tests native --gear spec .
190124-22:36:40,13 nipype.interface INFO:
	 We advise you to upgrade DIPY version. This upgrade will activate RecoBundles, LabelsBundles, DeterministicTracking.
2019-01-24 22:36:40,013 [    INFO] We advise you to upgrade DIPY version. This upgrade will activate RecoBundles, LabelsBundles, DeterministicTracking.
190124-22:36:40,22 nipype.interface INFO:
	 We advise you to upgrade DIPY version. This upgrade will activate DKIModel, MapmriModel, DTIModel, CSAModel, CSDModel.
2019-01-24 22:36:40,022 [    INFO] We advise you to upgrade DIPY version. This upgrade will activate DKIModel, MapmriModel, DTIModel, CSAModel, CSDModel.
2019-01-24 22:36:40,138 [    INFO] Creating gear for <class 'nipype.interfaces.ants.segmentation.LaplacianThickness'>
2019-01-24 22:36:40,339 [    INFO] Building gear docker image gearificator/nipype-interfaces-ants-segmentation-laplacianthickness:0.2.dev1.nipype.1.1.8.g597c03f64
2019-01-24 22:36:42,336 [    INFO] nipype.interfaces.ants.segmentation.LaplacianThickness gear generated
2019-01-24 22:36:42,338 [    INFO] TESTS: found 4 tests
2019-01-24 22:36:43,143 [    INFO]  test #1: defaults passed
2019-01-24 22:36:43,433 [   ERROR]  test #2: prior_thickness FAILED: Running ./run under ./tests-run/nipype/ants/segmentation/LaplacianThickness/prior_thickness failed. Exit: 1. See ./tests-run/nipype/ants/segmentation/LaplacianThickness/prior_thickness/logs/err
2019-01-24 22:36:44,087 [    INFO]  test #3: simple1 passed
2019-01-24 22:36:44,730 [    INFO]  test #4: smooth_param passed
gearificator --pdb spec process --regex '.*\.LaplacianTh' --run-tests native   3.59s user 1.72s system 92% cpu 5.752 total
(dev) 1 16763.....................................:Thu 24 Jan 2019 10:36:44 PM EST:.
(git-annex)hopa:~/proj/flywheel/gearificated-nipype[master]git
$> cat ./tests-run/nipype/ants/segmentation/LaplacianThickness/prior_thickness/logs/err
2019-01-24 22:36:43,405 [   ERROR] Error while running <nipype.interfaces.ants.segmentation.LaplacianThickness object at 0x7fac3fd0e610>: LaplacianThickness requires a value for input 'prior_thickness' because one of smooth_param is set. For a list of required inputs, see LaplacianThickness.help()
Traceback (most recent call last):
  File "/usr/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/home/yoh/proj/flywheel/gearificator/gearificator/__main__.py", line 3, in <module>
    main()
  File "/home/yoh/proj/flywheel/gearificator/gearificator/run.py", line 324, in main
    out = run(manifest, config, indir, outdir)
  File "/home/yoh/proj/flywheel/gearificator/gearificator/run.py", line 127, in run
    out = interface.run()
  File "/home/yoh/deb/gits/pkg-exppsy/nipype/nipype/interfaces/base/core.py", line 327, in run
    self._check_mandatory_inputs()
  File "/home/yoh/deb/gits/pkg-exppsy/nipype/nipype/interfaces/base/core.py", line 241, in _check_mandatory_inputs
    self._check_requires(spec, name, getattr(self.inputs, name))
  File "/home/yoh/deb/gits/pkg-exppsy/nipype/nipype/interfaces/base/core.py", line 209, in _check_requires
    raise ValueError(msg)
ValueError: LaplacianThickness requires a value for input 'prior_thickness' because one of smooth_param is set. For a list of required inputs, see LaplacianThickness.help()

pushed merge + changes. Any chance for it to get into #2864 ? ;-)

effigies · 2019-01-25T03:57:08Z

Any chance for it to get into #2864 ?

Yup. You'll want to merge master to make sure tests pass.

codecov-io · 2019-01-25T04:01:49Z

Codecov Report

Merging #2848 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #2848     +/-   ##
=========================================
+ Coverage   67.47%   67.58%   +0.1%     
=========================================
  Files         342      343      +1     
  Lines       43559    43666    +107     
  Branches     5422     5429      +7     
=========================================
+ Hits        29392    29512    +120     
+ Misses      13463    13443     -20     
- Partials      704      711      +7

Flag	Coverage Δ
#smoketests	`50.48% <0%> (-0.02%)`	⬇️
#unittests	`65.02% <100%> (+0.13%)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/ants/segmentation.py	`73.73% <ø> (ø)`	⬆️
nipype/interfaces/base/core.py	`86.88% <100%> (+0.07%)`	⬆️
nipype/utils/provenance.py	`84.07% <0%> (-0.64%)`	⬇️
nipype/interfaces/io.py	`55.07% <0%> (ø)`	⬆️
nipype/interfaces/freesurfer/preprocess.py	`66.11% <0%> (ø)`	⬆️
nipype/interfaces/dynamic_slicer.py	`17.47% <0%> (ø)`	⬆️
nipype/interfaces/nipy/preprocess.py	`45.79% <0%> (ø)`	⬆️
nipype/interfaces/workbench/__init__.py	`100% <0%> (ø)`	⬆️
nipype/interfaces/workbench/cifti.py	`100% <0%> (ø)`
nipype/pipeline/plugins/legacymultiproc.py	`66% <0%> (+4.5%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37e48ec...2ff466e. Read the comment docs.

yarikoptic · 2019-01-25T04:02:13Z

damn pushed didn't do the right thing, now pushed for sure, master was merged

effigies

You also need to make specs and commit the relevant changes.

nipype/interfaces/ants/tests/test_segmentation.py

effigies · 2019-01-28T15:26:06Z

@yarikoptic Do you have time to finish this up today?

yarikoptic · 2019-01-29T04:18:35Z

hm, I need to figure out where all the @mentions to me fall into my mailbox for nipype -- didn't see any and thus missed the deadline :`-( If you get a chance to progress on adding the test @effigies - would be appreciated!

Test fixed

effigies

@yarikoptic I fixed up your test. Can you review to make sure everything's how you want it?

effigies · 2019-02-14T15:32:53Z

nipype/interfaces/base/core.py

+                msg = fmt % (self.__class__.__name__,
+                             ', '.join("'%s'" % req for req in spec.requires),
+                             name,
+                             self.__class__.__name__)


I'm pretty sure this logic was backwards. Does anybody want to double-check me?

eyeballed it -- seems to produce logically correct statement at this moment, so all is good ;)

…unning tools/checkspecs.py

yarikoptic · 2019-02-15T19:27:00Z

pushed the autogenerated test (if autogenerated -- would be nice if there was a nipype-bot which would just generate/update them in the PR, like conda does ;)). Thanks @effigies for all other tuneups. Hopefully now it would be ready

effigies · 2019-02-15T19:29:54Z

There is a nipybot account. If someone knows how to write a bot where we can do that, I'm happy to share the login info.

1.1.9 (February 25, 2019) Full changelog: https://github.com/nipy/nipype/milestone/30?closed=1 * FIX: Make positional arguments to LaplacianThickness require previous argument (nipy#2848) * FIX: Import math and csv modules for bids_gen_info (nipy#2881) * FIX: Ensure outputs can be listed in camino.ProcStreamlines by defining instance variable (nipy#2739) * ENH: Allow afni.MaskTool to take multiple input files (nipy#2892) * ENH: Add flags dictionary input to spm.Level1Design (nipy#2861) * ENH: Threshold stddev once only in TSNR (nipy#2883) * ENH: Add workbench.CiftiSmooth interface (nipy#2871) * DOC: Replace initialism typo in comment with intended phrase (nipy#2875) * DOC: Fix typos in ANTs Registration input documentation (nipy#2869) * tag '1.1.9': (34 commits) MNT: Update changelog MNT: Add Katherine Bottenhorn, Paul Mihai to Zenodo MNT: Add kchawla-pi to Zenodo, update mailmap and ordering add to zenodo MNT: Update zenodo ordering Update .zenodo.json afni utils.py - masktool - InputMultiPath for in_file argument MNT: Update .zenodo ordering MNT: Add Oliver Contier name to .zenodo.json Update nipype/interfaces/spm/model.py ENH: Add zenodo updating script MNT: Update mailmap to avoid renames in script MNT: Update .mailmap, .zenodo.json MNT: Version 1.1.9 DOC: 1.1.9 changelog ENH: minor - compute non degenerate stddev map once BF: regenerated test_auto_LaplacianThickness using wonderfully long running tools/checkspecs.py TEST: Thorough test of LaplacianThickness requirement cascade FIX: Requires error text was backwards import math and csv modules for bids_gen_info ...

yarikoptic added 2 commits January 11, 2019 15:51

RF+ENH(TST): use %s instead of %f when rendering cmdline float point …

0430c85

…args This way makes it easier to test cmdline formation so we have 4.9 and not 4.90000

TST(BK): test_LaplacianThickness_wrongargs to demonstrate nipy#2847

fc18c78

effigies added this to the 1.1.8 milestone Jan 13, 2019

effigies mentioned this pull request Jan 25, 2019

REL: 1.1.8 #2864

Merged

16 tasks

yarikoptic added a commit to yarikoptic/gearificated-nipype that referenced this pull request Jan 25, 2019

ants.LaplacianThickness with a set of tests which demonstrate the pro…

de68c01

…blem of positional args see e.g. nipy/nipype#2848

BF: provide chain of requires for LaplacianThickness

d633a57

Thanks @effigies. This should prevent incorrect specification` of parameters in the command line since for ants they are just positional ones, so all previous ones should be specified

effigies previously requested changes Jan 27, 2019

View reviewed changes

nipype/interfaces/ants/tests/test_segmentation.py Outdated Show resolved Hide resolved

effigies modified the milestones: 1.1.8, 1.1.9 Jan 28, 2019

effigies added 2 commits February 14, 2019 10:31

FIX: Requires error text was backwards

56b2227

TEST: Thorough test of LaplacianThickness requirement cascade

b706af8

effigies approved these changes Feb 14, 2019

View reviewed changes

effigies changed the title ~~WiP BF 2847: must not incorrectly place positional args whenever prior ones aren't defined~~ FIX: Make positional arguments to LaplacianThickness require previous argument Feb 14, 2019

BF: regenerated test_auto_LaplacianThickness using wonderfully long r…

2ff466e

…unning tools/checkspecs.py

effigies merged commit 469c99c into nipy:master Feb 15, 2019

yarikoptic deleted the bf-2847 branch February 17, 2019 03:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Make positional arguments to LaplacianThickness require previous argument #2848

FIX: Make positional arguments to LaplacianThickness require previous argument #2848

yarikoptic commented Jan 11, 2019 •

edited by effigies

Loading

effigies commented Jan 13, 2019

effigies commented Jan 16, 2019

yarikoptic commented Jan 25, 2019

effigies commented Jan 25, 2019

codecov-io commented Jan 25, 2019 •

edited

Loading

yarikoptic commented Jan 25, 2019

effigies left a comment

effigies commented Jan 28, 2019

yarikoptic commented Jan 29, 2019

effigies left a comment

effigies Feb 14, 2019

yarikoptic Feb 15, 2019

yarikoptic commented Feb 15, 2019

effigies commented Feb 15, 2019

FIX: Make positional arguments to LaplacianThickness require previous argument #2848

FIX: Make positional arguments to LaplacianThickness require previous argument #2848

Conversation

yarikoptic commented Jan 11, 2019 • edited by effigies Loading

effigies commented Jan 13, 2019

effigies commented Jan 16, 2019

yarikoptic commented Jan 25, 2019

effigies commented Jan 25, 2019

codecov-io commented Jan 25, 2019 • edited Loading

Codecov Report

yarikoptic commented Jan 25, 2019

effigies left a comment

Choose a reason for hiding this comment

effigies commented Jan 28, 2019

yarikoptic commented Jan 29, 2019

effigies left a comment

Choose a reason for hiding this comment

effigies Feb 14, 2019

Choose a reason for hiding this comment

yarikoptic Feb 15, 2019

Choose a reason for hiding this comment

yarikoptic commented Feb 15, 2019

effigies commented Feb 15, 2019

yarikoptic commented Jan 11, 2019 •

edited by effigies

Loading

codecov-io commented Jan 25, 2019 •

edited

Loading